Skip to content

Stop using ob functions to echo pro global settings#2743

Merged
Crabcyborg merged 1 commit into
masterfrom
stop_using_ob_functions_to_echo_pro_global_settings
Jan 8, 2026
Merged

Stop using ob functions to echo pro global settings#2743
Crabcyborg merged 1 commit into
masterfrom
stop_using_ob_functions_to_echo_pro_global_settings

Conversation

@Crabcyborg

@Crabcyborg Crabcyborg commented Jan 8, 2026

Copy link
Copy Markdown
Contributor

$more_html is never used, so there's no point in not just triggering do_action.

@Crabcyborg Crabcyborg added this to the 6.27 milestone Jan 8, 2026
@Crabcyborg
Crabcyborg merged commit a7fdb32 into master Jan 8, 2026
34 of 36 checks passed
@Crabcyborg
Crabcyborg deleted the stop_using_ob_functions_to_echo_pro_global_settings branch January 8, 2026 17:55
@coderabbitai

coderabbitai Bot commented Jan 8, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The PR removes the $more_html parameter from the FrmSettingsController::maybe_render_currency_selector() method signature and updates the corresponding call site in the general settings view. Output buffering logic in the view is also removed.

Changes

Cohort / File(s) Summary
Controller Method Signature Update
classes/controllers/FrmSettingsController.php		 Updated maybe_render_currency_selector() method signature to remove the $more_html parameter; removed corresponding @param docblock entry.
View Call Site Update
classes/views/frm-settings/general.php		 Removed output buffering (ob_start(), ob_get_clean(), echo) around the "Other" section; updated maybe_render_currency_selector() call to pass only $frm_settings parameter instead of ($frm_settings, $more_html).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately describes the main change: removing output buffering functions to simplify the pro global settings echo logic.
Description check ✅ Passed The description clearly explains the rationale for the change: the $more_html parameter was unused, so output buffering was unnecessary and a do_action trigger could be used directly instead.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73f68f6 and 8f0d1cc.

📒 Files selected for processing (2)
  • classes/controllers/FrmSettingsController.php
  • classes/views/frm-settings/general.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/views/frm-settings/general.php (1)
classes/controllers/FrmSettingsController.php (2)
  • FrmSettingsController (6-540)
  • maybe_render_currency_selector (292-300)
🪛 PHPMD (2.15.0)
classes/controllers/FrmSettingsController.php

292-292: Avoid unused parameters such as '$frm_settings'. (undefined)

(UnusedFormalParameter)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: PHP 7.4 tests in WP 6.9
  • GitHub Check: PHP 8 tests in WP 6.9
  • GitHub Check: Cypress
  • GitHub Check: Cypress
  • GitHub Check: PHP 7.4 tests in WP 6.9
  • GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (2)
classes/views/frm-settings/general.php (1)

36-45: LGTM! Simplified call removes unnecessary complexity.

The updated call correctly matches the new method signature. The removal of output buffering (as mentioned in the PR description) simplifies the code since $more_html was never actually used. The do_action('frm_settings_form', $frm_settings) on line 42 remains unchanged, ensuring Pro add-ons can still hook into the settings rendering.

classes/controllers/FrmSettingsController.php (1)

292-300: No issues found. The method signature change is safe and the code is correct.

The single call to this method in the codebase (general.php line 44) already uses the new signature with one parameter, and $more_html has no references elsewhere. Additionally, $frm_settings is not unused—it's consumed by the included template (_currency.php) which accesses $frm_settings->currency. This is a standard PHP pattern where included templates have access to parent scope variables.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

stephywells pushed a commit that referenced this pull request Apr 4, 2026
@Crabcyborg
Merge pull request #2743 from Strategy11/stop_using_ob_functions_to_e…
0af1e10 
…cho_pro_global_settings

Stop using ob functions to echo pro global settings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

run analysis run tests

Projects

None yet

Milestone

6.27

Development

Successfully merging this pull request may close these issues.

1 participant

@Crabcyborg